Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX QuerySort::sort method should support sorting for multi arguments #563

Conversation

sabina-talipova
Copy link
Contributor

Description

Even if a method receives several parameters, then sorting occurs by the field of the arguments that is last specified in the schema, and not in the passed parameters. That is, if SilverStripe\GraphQL\Tests\Fake\DataObjectFake has the following schema

fields:
    myField: true
    AuthorID: true

and sorting is required AuthorID => DESC, myField => ASC, then sorting was done only by AuthorID, since in the schema this field is indicated after myField.
Therefore, the call $list->sort() must be completed after all the necessary processing has been carried out.

Parent issue

@@ -631,6 +668,62 @@ public function testFieldAliases()
$this->assertResult('readOneDataObjectFake.author', null, $result);
}

public function provideFilterAndSortOnlyRead(): array
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add one more test case for SortPlugin here.

@sabina-talipova sabina-talipova force-pushed the pulls/4.3/fix-broken-sort-order branch 7 times, most recently from 5f2f79c to 64bdd35 Compare November 23, 2023 09:31
@@ -7,7 +7,7 @@
"php": "^7.4 || ^8.0",
"silverstripe/framework": "^4.11",
"silverstripe/vendor-plugin": "^1.0",
"webonyx/graphql-php": "^14.0",
"webonyx/graphql-php": "^14.11",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this to pass --prefer-lowest CI. Since in 14.0.0 FieldDefinition doesn't have getName() method

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good, just a couple small things to tidy up

src/Schema/DataObject/Plugin/QuerySort.php Outdated Show resolved Hide resolved
src/Schema/DataObject/Plugin/QuerySort.php Outdated Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/4.3/fix-broken-sort-order branch from 64bdd35 to 5781c46 Compare November 23, 2023 21:09
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, tests are green here and were green when running against CMS 5 CI as well.

@GuySartorelli GuySartorelli merged commit 3db0b0f into silverstripe:4.3 Nov 23, 2023
12 checks passed
@GuySartorelli GuySartorelli deleted the pulls/4.3/fix-broken-sort-order branch November 23, 2023 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants